Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a security gating system for remote (HTTP/HTTPS) ES module loading in production builds. The feature allows developers to enable remote module imports via a security configuration section, with optional URL allowlisting for restricting module sources.
Key Changes:
- Added security configuration support in
nativescript.config(package.json) withallowRemoteModulesflag and optionalremoteModuleAllowlist - Implemented security checks at all HTTP module loading points in the runtime
- Added comprehensive test suite for security behavior
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| TestRunner/app/tests/index.js | Added import for new RemoteModuleSecurityTests |
| TestRunner/app/tests/RemoteModuleSecurityTests.js | New comprehensive test suite covering debug mode behavior, security configuration, URL allowlist matching, and edge cases |
| TestRunner/app/package.json | Added security configuration with allowRemoteModules flag and allowlist for testing |
| NativeScript/runtime/DevFlags.h | Added public API declarations for security config functions (IsRemoteModulesAllowed, IsRemoteUrlAllowed, InitializeSecurityConfig) |
| NativeScript/runtime/DevFlags.mm | Implemented security configuration parsing and URL allowlist prefix matching logic |
| NativeScript/runtime/ModuleInternalCallbacks.mm | Integrated security checks at all HTTP module loading entry points (absolute URLs, relative URLs from HTTP referrers, embedded URLs, dynamic imports) |
Comments suppressed due to low confidence (1)
NativeScript/runtime/ModuleInternalCallbacks.mm:1202
- Bug: After throwing a security exception at line 1165, the lambda returns
false, which allows execution to continue at line 1204 and beyond. This is inconsistent with other security checks in this file (e.g., lines 746-753, 875-882, 1050-1057) which immediately return an emptyMaybeLocal<v8::Module>()after throwing. The function should check if an exception was thrown and return immediately, or the lambda should signal an error differently so the caller at line 1191 can detect it and return early.
// Security check: block if remote modules not allowed
if (!IsHttpUrlAllowedForLoading(tail)) {
if (IsScriptLoadingLogEnabled()) {
Log(@"[resolver][security] blocked embedded remote module: %s", tail.c_str());
}
std::string msg = GetRemoteModuleBlockedMessage(tail);
isolate->ThrowException(v8::Exception::Error(tns::ToV8String(isolate, msg.c_str())));
return false;
}
if (IsScriptLoadingLogEnabled()) { Log(@"[resolver][http-embedded] %s -> %s", p.c_str(), tail.c_str()); }
std::string key = CanonicalizeHttpUrlKey(tail);
auto itExisting = g_moduleRegistry.find(key);
if (itExisting != g_moduleRegistry.end()) {
v8::Local<v8::Module> existing = itExisting->second.Get(isolate);
if (!existing.IsEmpty() && existing->GetStatus() != v8::Module::kErrored) {
return !v8::MaybeLocal<v8::Module>(existing).IsEmpty();
}
}
std::string body; std::string ct; int status = 0;
if (HttpFetchText(tail, body, ct, status) && !body.empty()) {
v8::MaybeLocal<v8::Module> m = CompileModuleForResolveRegisterOnly(isolate, context, body, key);
if (!m.IsEmpty()) {
v8::Local<v8::Module> mod; if (m.ToLocal(&mod)) { return true; }
}
}
if (RuntimeConfig.IsDebug) {
std::string msg = "HTTP embedded import failed: " + tail;
isolate->ThrowException(v8::Exception::Error(tns::ToV8String(isolate, msg.c_str())));
}
return false;
};
if (rerouteHttpIfEmbedded(absPath)) {
// Return the module from registry; V8 will pick it up
std::string key = CanonicalizeHttpUrlKey(absPath.substr(absPath.find("http")));
auto itExisting = g_moduleRegistry.find(key);
if (itExisting != g_moduleRegistry.end()) {
v8::Local<v8::Module> existing = itExisting->second.Get(isolate);
if (!existing.IsEmpty()) {
return v8::MaybeLocal<v8::Module>(existing);
}
}
// If not present, fall through to normal checks
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security gating system for remote (HTTP/HTTPS) ES module loading in production.
securitysectionnativescript.config, enabling remote modules and specifying an allowlist of URL prefixes for permitted remote module sources.In development, defaults to always enabled.
In production, only enabled via explicit developer approval in nativescript.config settings:
If the optional
remoteModuleAllowlistis omitted, it allows any url.Docs added: https://github.com/NativeScript/docs/pull/191/files